Draft: Add Puppet proxy (CA) bulk actions #429

nadjaheitmann · 2025-09-24T10:52:47Z

@adamruzicka @jeremylenz I have created a draft for the Puppet Proxy update bulk action. It is not more than a draft and I know there is a bunch of things missing (e.g. clearing proxy, CA proxy, tests, ...). I basically took code from Foreman and Katello and mixed it into this. Do you mind checking whether the general approach is the direction we want to go. UI wise, I think this is straight forward, but I was not sure whether

extending the BulkHostManager
adding a new API endpoint to the HostsBulkActionsController
adding the API endpoint itself to the Foreman rather than the foreman_puppet namespace

is the way to go.

I appreciate your opinions.

adamruzicka

Left some notes inline, can't speak much to the frontend

adamruzicka · 2025-09-24T11:58:54Z

app/controllers/concerns/foreman_puppet/extensions/api_v2_hosts_bulk_actions_controller.rb

+      extend ActiveSupport::Concern
+
+      included do
+        before_action :find_editable_hosts, :only => [:change_puppet_proxy]


Won't this override the before action from the base class in https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/hosts_bulk_actions_controller.rb#L8 ?

In this case I'd prefer katello's approach - have a brand new controller rather than trying to extend the existing one.

Yeah, it does. Is there a way not to overwrite it? If not, it has to be a new controller, indeed.

From the top of my head I can't suggest any clean way that would make it behave the way we need. Hacks could be done, sure, with the cleanest one probably being just calling it directly in #change_puppet_proxy without relying on controller callbacks.

adamruzicka · 2025-09-24T12:01:14Z

app/controllers/concerns/foreman_puppet/extensions/api_v2_hosts_bulk_actions_controller.rb

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


This being required means it can be used to reassign the host to a different proxy, but this cannot be used to unassign it, correct?

Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

adamruzicka · 2025-09-24T12:02:31Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Do we need callbacks to be run or could we take a shortcut and assign the proxy with a single update query?

Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

adamruzicka · 2025-09-24T12:10:28Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+        rescue StandardError => e
+            message = format(_('Failed to set %{proxy_type} proxy for %{host}.'), host: host, proxy_type: proxy_type)
+            Foreman::Logging.exception(message, e)
+        end
+        rescue RecordNotFoundError => e
+          Foreman::Logging.exception(_('Cound not find Smart Proxy'))
+        end


Suggested change

rescue StandardError => e

message = format(_('Failed to set %{proxy_type} proxy for %{host}.'), host: host, proxy_type: proxy_type)

Foreman::Logging.exception(message, e)

end

rescue RecordNotFoundError => e

Foreman::Logging.exception(_('Cound not find Smart Proxy'))

end

rescue StandardError => e

message = format(_('Failed to set %{proxy_type} proxy for %{host}.'), host: host, proxy_type: proxy_type)

Foreman::Logging.exception(message, e)

end

rescue ActiveRecord::RecordNotFound => e

Foreman::Logging.exception(_('Cound not find Smart Proxy'))

end

And even then, ActiveRecord::RecordNotFound is a subclass of StandardError so the record not found branch would never match

Yeah, that one is messy - this also somehow depends if we want a separate method for unassign.

adamruzicka · 2025-09-24T12:11:45Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+      extend ActiveSupport::Concern
+
+      def change_puppet_proxy(proxy_id, ca_proxy)
+        proxy = SmartProxy.find_by(id: proxy_id)


This just returns nil if no proxy was found. Is that expected?

Yes. I would solve that depending on how I implement the unassign method. The PR is still very drafty.

nadjaheitmann

Thanks @adamruzicka !

nadjaheitmann · 2025-09-24T12:19:51Z

app/controllers/concerns/foreman_puppet/extensions/api_v2_hosts_bulk_actions_controller.rb

+      extend ActiveSupport::Concern
+
+      included do
+        before_action :find_editable_hosts, :only => [:change_puppet_proxy]


Yeah, it does. Is there a way not to overwrite it? If not, it has to be a new controller, indeed.

nadjaheitmann · 2025-09-24T12:21:18Z

app/controllers/concerns/foreman_puppet/extensions/api_v2_hosts_bulk_actions_controller.rb

+
+        api :PUT, "/hosts/bulk/change_puppet_proxy", N_("Change Puppet Proxy")
+        param_group :bulk_host_ids
+        param :proxy_id, :number, :required => true, :desc => N_("ID of the Puppet proxy to reassign the hosts to")


Correct. I have not yet implemented the unassign method, so the implementation details about that one are still open.

nadjaheitmann · 2025-09-24T12:23:37Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+      extend ActiveSupport::Concern
+
+      def change_puppet_proxy(proxy_id, ca_proxy)
+        proxy = SmartProxy.find_by(id: proxy_id)


Yes. I would solve that depending on how I implement the unassign method. The PR is still very drafty.

nadjaheitmann · 2025-09-24T12:28:00Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+              host.puppet_ca_proxy = proxy
+            else
+              host.puppet_proxy = proxy
+            end
+            host.save(:validate => false)


Not sure, if we need callbacks to be honest. The original action does run some validations about validating the proxy I think: https://github.com/theforeman/foreman_puppet/blob/master/app/controllers/concerns/foreman_puppet/extensions/hosts_controller_extensions.rb#L143

Can the update command also take the validate => false parameter?

nadjaheitmann · 2025-09-24T12:30:33Z

app/services/concerns/foreman_puppet/extensions/bulk_hosts_manager.rb

+        rescue StandardError => e
+            message = format(_('Failed to set %{proxy_type} proxy for %{host}.'), host: host, proxy_type: proxy_type)
+            Foreman::Logging.exception(message, e)
+        end
+        rescue RecordNotFoundError => e
+          Foreman::Logging.exception(_('Cound not find Smart Proxy'))
+        end


Yeah, that one is messy - this also somehow depends if we want a separate method for unassign.

pr-processor bot added the Not yet reviewed label Sep 24, 2025

adamruzicka reviewed Sep 24, 2025

View reviewed changes

pr-processor bot removed the Not yet reviewed label Sep 24, 2025

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from e2b1c7f to c7dd83c Compare September 24, 2025 12:15

nadjaheitmann commented Sep 24, 2025

View reviewed changes

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from ac62ab7 to e980546 Compare September 24, 2025 12:48

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 3 times, most recently from 11c8919 to 4512e86 Compare October 13, 2025 07:52

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 4512e86 to 2eb2f1a Compare January 21, 2026 17:09

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch 2 times, most recently from c057e40 to 4bb7ef6 Compare February 9, 2026 13:16

Add Puppet proxy (CA) bulk actions

0faf7d6

nadjaheitmann force-pushed the new_all_hosts_page_puppet_proxy_actions branch from 4bb7ef6 to 0faf7d6 Compare February 10, 2026 10:44

Draft: Add Puppet proxy (CA) bulk actions #429

Are you sure you want to change the base?

Draft: Add Puppet proxy (CA) bulk actions #429

Uh oh!

Conversation

nadjaheitmann commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadjaheitmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nadjaheitmann commented Sep 24, 2025 •

edited

Loading